Skip to content

Fix ReachTargetVoltage handling for too small resistance#2290

Merged
t-b merged 9 commits intomainfrom
bugfix/2290-reach-target-voltage
Jan 16, 2025
Merged

Fix ReachTargetVoltage handling for too small resistance#2290
t-b merged 9 commits intomainfrom
bugfix/2290-reach-target-voltage

Conversation

@t-b
Copy link
Copy Markdown
Collaborator

@t-b t-b commented Oct 25, 2024

We used to always use 100pA if the resistance is tool small, but the code obviously only wanted to do that only on the first sweep. Fix that.

@t-b t-b mentioned this pull request Nov 5, 2024
Merged
48 tasks
@t-b t-b force-pushed the bugfix/2290-reach-target-voltage branch 3 times, most recently from 25b0267 to dbc993d Compare November 19, 2024 10:59
@t-b t-b mentioned this pull request Jan 6, 2025
Closed
3 tasks
@timjarsky
Copy link
Copy Markdown
Collaborator

timjarsky commented Jan 9, 2025 •
edited
Loading

Acquisition continues after required sweeps have been acquired. It's been a while, but I recall it should stop automatically.

  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

2025_01_09_122405_2.zip

@timjarsky timjarsky assigned t-b and unassigned timjarsky Jan 9, 2025
t-b added 9 commits January 14, 2025 15:51
@t-b
tests: Factor MarkDeviceAsLocked out
021c82d
@t-b
Packages/tests: Add ConstantZero stimset
c38d189
@t-b
HARDWARE_ITC_BITS_PER_VOLT: Enhance comment
362bb1b
@t-b
HW_GetVoltageRange: Rename it
5787333 
We don't return voltages for all hardware types, so the current name is
confusing.

So let's rename it.
@t-b
tests/InitDAQSettingsFromString: Check headstage
8698bdd
@t-b
ReachTargetVoltage: Prefer sweepNo from the structure
63e78c1 
Missed in 85d6c5b (ReachTargetVoltage: Convert to V3 analysis function, 2020-11-20).
@t-b
ReachTargetVoltage: Fix initial response check
6f0280e 
We want to only act specially on the very first sweep. But as we reset the
index, we will land in this branch on every sweep if the resistance is too
small.

Let's prefer comparing to the number of acquired sweeps instead.
@t-b
ReachTargetVoltage: Prefer scaledDACWave from the structure
fe090a4 
Missed in 85d6c5b (ReachTargetVoltage: Convert to V3 analysis function, 2020-11-20).
@t-b
Tests/UTF_ReachTargetVoltage.ipf: Rework them
10f81b5 
We know have 6 sweeps in the stimset, which triggers the finish condition
as we need one sweep more than the targetVoltages wave. Also added is a
test with multiple headstages.
@t-b
Copy link
Copy Markdown
Collaborator Author

t-b commented Jan 14, 2025 •
edited
Loading

Acquisition continues after required sweeps have been acquired. It's been a while, but I recall it should stop automatically.

That's the "new" approach we do in the modern analysis functions. ReachTargetVoltage was one of the first ones (2017) and does not follow that approach. We can fix that but this is IMHO tangent to the fix in this PR.

In your pxp I see in the history some bug messages

 MIES BUG_TS: Encountered pending RTE: 1321, a wave read;Index out of range for wave "_free_".
  MIES BUG_TS: Encountered pending RTE: 1321, a wave read;Index out of range for wave "_free_".
  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.
  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

which need fixing. I tried to reproduce that here, but that did not work. So I would need a screen share for that.

@t-b t-b force-pushed the bugfix/2290-reach-target-voltage branch from dbc993d to 10f81b5 Compare January 14, 2025 18:30
@timjarsky
Copy link
Copy Markdown
Collaborator

timjarsky commented Jan 15, 2025

@t-b I also get the error in main, so I guess we designed it such that the number of sweeps needs to match the size of DAScales (not an analysis param).

  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

@timjarsky timjarsky removed their assignment Jan 15, 2025
@t-b t-b merged commit 86bdadf into main Jan 16, 2025
@t-b t-b deleted the bugfix/2290-reach-target-voltage branch January 16, 2025 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timjarsky timjarsky timjarsky approved these changes

@MichaelHuth MichaelHuth Awaiting requested review from MichaelHuth MichaelHuth is a code owner

Assignees

@t-b t-b

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@t-b @timjarsky